Codecov Report

Attention: Patch coverage is 94.35897% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.03%. Comparing base (949e7db) to head (993ed08).
Report is 7 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1741      +/-   ##
==========================================
- Coverage   90.04%   90.03%   -0.02%     
==========================================
  Files         396      396              
  Lines       37928    37968      +40     
  Branches     4262     4261       -1     
==========================================
+ Hits        34154    34183      +29     
- Misses       2477     2491      +14     
+ Partials     1297     1294       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thank you very much,
I played around a bit and learned some things.
For the future, please tell me, which targets you compiled (I assume benchmark among others), or whether you manually compiled even single header files etc. Otherwise I also get many errors of things that would have been easy to fix.

I update your branch, so make sure to git pull and have a look at my changes first:

• General:

1. Inside a CPP_template_def you have to use CPP_and_def (which I just implemented) instead of CPP_and
2. Inside a CPP_template(_def) you have to use CPP_NOT(someConstraint) instead of !someConstraint.
   (I will also add those two points to the Wiki)

• SerializeVector.h ends up with an incomplete type when building with C++17 flag
  I found some issues with the AD_SERIALIZE_FUNCTION_WITH_CONSTRAINT macros (in particular they currently don't support commas inside their arguments, I worked around for some of them, have a look and see if you can fix your problem in a similar way.

• ConfigOptionProxy.h seems to not forward the underlying type correctly, causing the ValidatorFunction constraints to fail when using the addValidator method from ConfigOption.h (See commented code in BenchmarkExamples.cpp)
  This works now (the problem was, that std::invoke_result_t is not at all SFINAE-friendly, but still works with C++20 concepts. See my changes in TypeTraits.h for details.

• A solution has to be found for the isInstantiation constraints imposed in some parts of ConfigOption.h, which I currently had to remove to make it build with v3-ranges.

I have fixed the isInstantiation and (as a proof of concept) readded it to the first occurence in ConfigOption.h, feel free to extend it to the remaining occurences.

I hope that with these changes and suggestions you can continue, please let me know when there is additional trouble, then we can communicate further (probably next week).

Thank you very much, I played around a bit and learned some things. For the future, please tell me, which targets you compiled (I assume benchmark among others), or whether you manually compiled even single header files etc. Otherwise I also get many errors of things that would have been easy to fix.

I update your branch, so make sure to git pull and have a look at my changes first:

• General:

1. Inside a CPP_template_def you have to use CPP_and_def (which I just implemented) instead of CPP_and
2. Inside a CPP_template(_def) you have to use CPP_NOT(someConstraint) instead of !someConstraint.
   (I will also add those two points to the Wiki)

• SerializeVector.h ends up with an incomplete type when building with C++17 flag
  I found some issues with the AD_SERIALIZE_FUNCTION_WITH_CONSTRAINT macros (in particular they currently don't support commas inside their arguments, I worked around for some of them, have a look and see if you can fix your problem in a similar way.

• ConfigOptionProxy.h seems to not forward the underlying type correctly, causing the ValidatorFunction constraints to fail when using the addValidator method from ConfigOption.h (See commented code in BenchmarkExamples.cpp)
  This works now (the problem was, that std::invoke_result_t is not at all SFINAE-friendly, but still works with C++20 concepts. See my changes in TypeTraits.h for details.

• A solution has to be found for the isInstantiation constraints imposed in some parts of ConfigOption.h, which I currently had to remove to make it build with v3-ranges.

I have fixed the isInstantiation and (as a proof of concept) readded it to the first occurence in ConfigOption.h, feel free to extend it to the remaining occurences.

I hope that with these changes and suggestions you can continue, please let me know when there is additional trouble, then we can communicate further (probably next week).

Thank you! For reference, I build the whole engine just to make sure that everything still builds especially since I'm currently working on the utility library which is used throughout a good portion of the codebase.

Will let you know if I spot more problems.

Hello @joka921 ,
Updated the PR description with some remarks. Feedback would be appreciated. Thanks!

@gpicciuca
I have just fixed some issues, but others remain:

0. When using CPP_template the CPP_and must not be inside additional parentheses wrt the requires.
   So ... (requires ( something<T> CPP_and something<U>)) does not compile.

1. Variadic templates (template <typename...>) don't work with CPP_template. I have chosen QL_CONCEPT_OR_NOTHING in those cases.

2. Same goes for partial specializations (the issues in the json.h.

3. In the Views.h You have to use CPP_ret because you can't use CPP_template in member functions, when the outer templated class already uses CPP_template (Due to the internals of this macro). The disadvantage is, that for CPP_ret you need the exact return type. This is something where you could continue, I have started with the first occurence.

TLDR:
With things I have pushed you can compile the util library, The next step would be to compile ViewsTest .

In general you should work on changing one thing, and then making sure that it compiles in both modes, currently I am doing a lot of cleanup in different places where it would be possible to discuss the occuring problems in advance, but we are all just learning to use these macros and their limitations (I have also learned several things about template metaprogramming in the past week).
Let me know, when you are working on your branch, so we won't get in conflict.

@gpicciuca I have just fixed some issues, but others remain:

0. When using CPP_template the CPP_and must not be inside additional parentheses wrt the requires.
   So ... (requires ( something<T> CPP_and something<U>)) does not compile.
1. Variadic templates (template <typename...>) don't work with CPP_template. I have chosen QL_CONCEPT_OR_NOTHING in those cases.
2. Same goes for partial specializations (the issues in the json.h.
3. In the Views.h You have to use CPP_ret because you can't use CPP_template in member functions, when the outer templated class already uses CPP_template (Due to the internals of this macro). The disadvantage is, that for CPP_ret you need the exact return type. This is something where you could continue, I have started with the first occurence.

TLDR: With things I have pushed you can compile the util library, The next step would be to compile ViewsTest .

In general you should work on changing one thing, and then making sure that it compiles in both modes, currently I am doing a lot of cleanup in different places where it would be possible to discuss the occuring problems in advance, but we are all just learning to use these macros and their limitations (I have also learned several things about template metaprogramming in the past week). Let me know, when you are working on your branch, so we won't get in conflict.

0. Noted. All those issues are popping out as I build with the cpp17 flag. Easy fix.
1. I found a better solution for that which keeps the constraints in place for both builds. E.g.:

template <typename... Tuples>
using TupleCat =
   typename std::enable_if<(isTuple<Tuples> && ...),
                           decltype(std::tuple_cat(
                               std::declval<Tuples&>()...))>::type;

A bit more verbose, but it works.
2. For the json.h file I also have a working fix in place without sacrificing constraints. E.g. this one was giving me build errors but works now this way since the base struct definition allows for the second template argument

template <typename T>
struct adl_serializer<std::unique_ptr<T>,
                      std::enable_if_t<std::is_copy_constructible_v<T>>> {

3. Was about to experiment with that one. Thanks.

@joka921
I pushed few more changes among which there are a few small fixes that still make sure constraints are applied even in C++17 (where you went for QL_CONCEPT_... etc).

The changes in Views.h and Iterator.h are somewhat work in progress as I've been experimenting there but still no luck in getting through the errors. ViewsTest are still failing to compile even after applying your suggested approach with CPP_ret. Seems like this method actually changes how constraints are applied to the method. I also tried templating all of those methods applying SFINAE techniques with and without the v3-ranges macros, but still fails.
The issue is in how const-qualifications are propagated through OwningView, BufferedAsyncView, InputRangeMixin. For yet unknown reasons the const qualifications break after applying above mentioned changes, so it tries to call into a const begin() which it can't find.

Will continue working on this next week. Feel free to have a look in the meantime if you have time.

Okay, I have brought this back to compilation in both modes, but it took me quite a lot of time.
I reverted as part of my work most of the concepts back to concept from CPP_concept, as there were a lot of compilation errors, a few of them because of subtleties (for which it is absolutely fine to pass them on to me) and most of them because of "the necessary work was not yet done, but there was nothing special about it". I below propose a plan to mitigate the remaining issues in a productive way for the both of us:

Please continue as follows:

1. Pick a single concept (start e.g. in TypeTraits.h) and change it to CPP_concept.
2. Change all the places where this concept is used, s.t. both modes (C++20 and 17 ) compile again.
3. IMPORTANT. If you don't suceed for some places, because the usage is non-trivial or messy, change all the trivial places and then revert the concept back to concept, such that the code compiles again.
4. Repeat steps 1-3 for other concepts.

At some point, contact me again with the list of concepts where issues remain (the ones from point 3.) such that I can have a look at the more advanced techniques that we might need there.

Let me know, if I can be of assistance or if there are additional questions.

Ignore the force-push I just did please. Rebase went wrong. Working on it.

Rebase solved. Both modes build just fine with this last commit. Tackling each concept now individually to not end up in a dead-end again.

@gpicciuca
I am just seeing that you do a lot of manual rewriting using std::enable_if_t instead of using the range-v3 macros as discussed.
In some cases there are additional macros that you can use if the plain CPP_template(...) doesn't work.
Please let me know if you run into trouble before you do a lot of stuff that then has to be reverted again.

@gpicciuca I am just seeing that you do a lot of manual rewriting using std::enable_if_t instead of using the range-v3 macros as discussed. In some cases there are additional macros that you can use if the plain CPP_template(...) doesn't work. Please let me know if you run into trouble before you do a lot of stuff that then has to be reverted again.

I'm going through those enable_if_t templates again.
Some places I cannot simply rewrite with the macros, like the changes i made in benchmark/JoinAlgorithmBenchmark.cpp. It says you cannot define templates in block-scope. But the lambda per-se can be templated though..

One thing worth mentioning is that with the v3-ranges macros, you cannot have variadic arguments there.

For example:
In src/engine/sparqlExpressions/SparqlExpressionTypes.h

template <size_t NumOperands,
          QL_CONCEPT_OR_TYPENAME(
              ad_utility::isInstantiation<FunctionAndValueGetters>)
              FunctionAndValueGettersT,
          QL_CONCEPT_OR_TYPENAME(ad_utility::isInstantiation<
                                 SpecializedFunction>)... SpecializedFunctions>
struct Operation {

In these cases (and there are quite a few), we cannot even apply the normal C++17 sfinae techniques as you'd have to specify the enable_if_t at the end of the template argument list (meaning even after the variadic args).

Another example, although just a test:
In test/SparqlAntlrParserTest.cpp

auto matchNary(
    auto makeFunction,
    QL_CONCEPT_OR_NOTHING(ad_utility::SimilarTo<Variable>) auto&&... variables)
    -> ::testing::Matcher<const sparqlExpression::SparqlExpression::Ptr&> {

The CPP_template macro would append some other internal arguments at the end of your manually specified template args, leading to an invalid syntax which the compiler is unhappy about.

@joka921 Since you're working on this branch right now and my changes for the next PR are ready, is it fine to push here or shall I create a new PR and rebase afterwards (2nd PR is a branch-off from this one)?

@gpicciuca
Please wait until I am done here until pushing here.
I would suggest that I finish this here and merge it, and you then start a fresh branch and PR

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=993ed081a0869e42d096a5f82e2330f21aad35fb&prev=702339f7f6b408788e0dd95587a5bf34ed6ce036

Quality Gate passed

Issues
21 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud